Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[JENKINS-69869] Categorize the user properties #7268

Merged
merged 18 commits into from
Jul 11, 2024
Merged

[JENKINS-69869] Categorize the user properties #7268

merged 18 commits into from
Jul 11, 2024

Conversation

Wadeck
Copy link
Contributor

@Wadeck Wadeck commented Oct 16, 2022 •
edited by MarkEWaite
Loading

See JENKINS-69869.

This PR is a proposal to categorize the user properties. It follows the same idea as with the Global configuration pages.

Originally I was working on JENKINS-69853 but I wanted to put them in a separate page and end up creating this instead. That's also why you can see the UserPropertyCategory.Experimental as a WIP stuff.

If you test this PR live, you can look at the Experimental category to see the behavior in the case there is no enabled properties in a given category.

The category system is meant to be used by plugins without requiring to have the most recent version of core, with the String based method.

Screenshots

Before

Screenshot_2022-10-16_184650_001

After

image

If there is no enabled properties

image

For reviewers

This PR is structured to ease your review. You can review commit by commit to ease the understanding of the intent.

Structure of this PR in terms of commit:

  1. Logic change to support new categories (= main part of the PR)
  2. Update the existing properties in Jenkins core
  3. Copy-paste the translation files used for configure.jelly in User

Testing done

Proposed changelog entries

  • User properties are now categorized in different pages.

Proposed upgrade guidelines

N/A

Submitter checklist

  • The Jira issue, if it exists, is well-described.
  • [x]The changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developers, depending on the change) and are in the imperative mood (see examples).
    • Fill in the Proposed upgrade guidelines section only if there are breaking changes or changes that may require extra steps from users during upgrade.
  • There is automated testing or an explanation as to why this change has no tests.
  • New public classes, fields, and methods are annotated with @Restricted or have @since TODO Javadocs, as appropriate.
  • New deprecations are annotated with @Deprecated(since = "TODO") or @Deprecated(forRemoval = true, since = "TODO"), if applicable.
  • [n/a] New or substantially changed JavaScript is not defined inline and does not call eval to ease future introduction of Content Security Policy (CSP) directives (see documentation).
  • [n/a] For dependency updates, there are links to external changelogs and, if possible, full differentials.
  • For new APIs and extension points, there is a link to at least one consumer.

Desired reviewers

@daniel-beck @basil As we were quickly discussing about the feature flag topic, this could be a first step in that direction. But not required, as the flags could be a simple UserProperty.

Maintainer checklist

Before the changes are marked as ready-for-merge:

  • There are at least two (2) approvals for the pull request and no outstanding requests for change.
  • Conversations in the pull request are over, or it is explicit that a reviewer is not blocking the change.
  • Changelog entries in the pull request title and/or Proposed changelog entries are accurate, human-readable, and in the imperative mood.
  • Proper changelog labels are set so that the changelog can be generated automatically.
  • If the change needs additional upgrade steps from users, the upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the pull request title (see example).
  • If it would make sense to backport the change to LTS, a Jira issue must exist, be a Bug or Improvement, and be labeled as lts-candidate to be considered (see query).

@Wadeck Wadeck marked this pull request as draft October 16, 2022 20:23
@timja timja requested a review from janfaracik October 20, 2022 09:05
timja
timja reviewed Aug 20, 2023
View reviewed changes
Copy link
Member

@timja timja left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Wadeck are you interested in finishing this off?

It's definitely a great idea and a needed feature

Get "approval" about the direction

Looks good

Add a system property to keep the previous approach in place

Why?

(not sure about the process) After merge, updating the ATH to pass with the new approach

All ATH tests need to be passing pre-merge, tbh I'm not aware (as-in had to touch any, not searched) of any tests specifically that touch user config there may not be many to fix.

@timja timja added major-rfe For changelog: Major enhancement. Will be highlighted on the top web-ui The PR includes WebUI changes which may need special expertise developer Changes which impact plugin developers labels Aug 20, 2023
@janfaracik
Copy link
Contributor

+1 on this - I think it's a great idea. Apologies for not seeing it sooner.

@timja timja mentioned this pull request Aug 22, 2023
Merged
@Wadeck
Copy link
Contributor Author

Wadeck commented Aug 31, 2023

👋 Interested yes for sure, having time "soon", less sure :)
Anyone who would like to take over is very welcome, otherwise I will do it when time permits.

Add a system property to keep the previous approach in place

Why?

I really like the experience I have with some applications that proposes a way to "rollback" UI changes. If you think it's an overkill (in general or in this particular case), please tell me ;-)

@timja timja mentioned this pull request Jun 8, 2024
Merged
6 tasks
This was referenced Jun 8, 2024
Merged
Merged
@timja
Copy link
Member

timja commented Jun 8, 2024 •
edited
Loading

I've:

  • Refactored out common code
  • Polished user facing terminology
  • Removed the half done feature flagging for falling back to the old method
  • Adapted the merged feature flagging capability to this
  • Added appearance category
  • Adapted theme manager, custom header and sshd plugins

I'm happy with this UX and code wise now.

TODO:

  • fix any failing tests in core
  • run PCT - running again tests should be fixed though
  • run ATH

This was referenced Jun 9, 2024
Closed
Merged
Merged
@timja timja added the ath-successful This PR has successfully passed the full acceptance-test-harness suite label Jun 9, 2024
@timja timja marked this pull request as ready for review June 9, 2024 21:28
@janfaracik
Copy link
Contributor

Clicking the 'Configure' button on the Users (http://localhost:8080/jenkins/manage/securityRealm/) page results in a 404

@timja
Copy link
Member

timja commented Jun 21, 2024

@jenkinsci/core-security-review possible to get a review please?

@Kevin-CB
Copy link
Contributor

@jenkinsci/core-security-review possible to get a review please?

Yeah, we're aware of this PR and are planning to review it soonish

Kevin-CB
Kevin-CB approved these changes Jun 26, 2024
View reviewed changes
Copy link
Contributor

@Kevin-CB Kevin-CB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested this PR locally, and reviewed code, I don't see any security concern!

core/src/main/java/hudson/model/userproperty/UserPropertyCategoryAccountAction.java Show resolved Hide resolved
core/src/main/java/hudson/model/userproperty/UserPropertyCategoryAppearanceAction.java Show resolved Hide resolved
core/src/main/java/hudson/model/userproperty/UserPropertyCategoryExperimentalAction.java Show resolved Hide resolved
core/src/main/java/hudson/model/userproperty/UserPropertyCategoryPreferencesAction.java Show resolved Hide resolved
core/src/main/java/hudson/model/userproperty/UserPropertyCategorySecurityAction.java Show resolved Hide resolved
@Kevin-CB Kevin-CB added security-approved @jenkinsci/core-security-review reviewed this PR for security issues and removed needs-security-review Awaiting review by a security team member labels Jun 26, 2024
@timja
Copy link
Member

timja commented Jun 26, 2024

Thanks! Addressed and retested with an admin and read only user, read only user can now see their own everything and the basic ones that they were allowed to before of admins, and admin can see everything.

/label ready-for-merge

This PR is now ready for merge, after ~24 hours, we will merge it if there's no negative feedback.

Thanks!

@comment-ops-bot comment-ops-bot bot added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Jun 26, 2024
@timja timja merged commit 6fa26d2 into jenkinsci:master Jul 11, 2024
16 checks passed
@Wadeck
Copy link
Contributor Author

Wadeck commented Jul 11, 2024

Thanks @timja to have worked on this one <3

@basil
Copy link
Member

basil commented Jul 11, 2024

Thanks @timja! This is creating conflicts on the jakarta branch; could you please merge it there?

@timja
Copy link
Member

timja commented Jul 11, 2024

Done

@basil
Copy link
Member

basil commented Jul 11, 2024

Thanks @timja! We're back up-to-date again. 🙌

@basil
Copy link
Member

basil commented Jul 12, 2024

Hi @timja, unfortunately this is causing a new ATH failure in core.JenkinsDatabaseSecurityRealmTest#create_update_delete. I think that test needs to be updated to use account instead of configure as was done in this PR.

org.openqa.selenium.TimeoutException: Expected condition failed: Element matching By.xpath: //form[contains(@name, 'config')] is present (tried for 10 second(s) with 500 milliseconds interval)
	at org.openqa.selenium.support.ui.FluentWait.timeoutException(FluentWait.java:265)
	at org.jenkinsci.test.acceptance.junit.Wait.timeoutException(Wait.java:188)
	at org.openqa.selenium.support.ui.FluentWait.until(FluentWait.java:228)
	at org.jenkinsci.test.acceptance.junit.Wait.until(Wait.java:119)
	at org.jenkinsci.test.acceptance.po.CapybaraPortingLayerImpl.waitFor(CapybaraPortingLayerImpl.java:137)
	at org.jenkinsci.test.acceptance.po.ConfigurablePageObject.configure(ConfigurablePageObject.java:113)
	at core.JenkinsDatabaseSecurityRealmTest.create_update_delete(JenkinsDatabaseSecurityRealmTest.java:83)
	at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
	at java.base/java.lang.reflect.Method.invoke(Method.java:580)
	at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:59)
	at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
	at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:56)
	at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
	at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:26)
	at org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:27)
	at org.junit.rules.TestWatcher$1.evaluate(TestWatcher.java:61)
	at org.jenkinsci.test.acceptance.junit.WithPlugins$RuleImpl$1.evaluate(WithPlugins.java:160)
	at org.junit.rules.TestWatcher$1.evaluate(TestWatcher.java:61)
	at org.junit.rules.TestWatcher$1.evaluate(TestWatcher.java:61)
	at org.junit.rules.TestWatcher$1.evaluate(TestWatcher.java:61)
	at org.jenkinsci.test.acceptance.junit.JenkinsAcceptanceTestRule$1$1.evaluate(JenkinsAcceptanceTestRule.java:153)
	at org.jenkinsci.test.acceptance.junit.FilterRule$1.evaluate(FilterRule.java:61)
	at org.junit.rules.TestWatcher$1.evaluate(TestWatcher.java:61)
	at org.jenkinsci.test.acceptance.junit.JenkinsAcceptanceTestRule$1.evaluate(JenkinsAcceptanceTestRule.java:53)
	at org.junit.runners.ParentRunner$3.evaluate(ParentRunner.java:306)
	at org.junit.runners.BlockJUnit4ClassRunner$1.evaluate(BlockJUnit4ClassRunner.java:100)
	at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:366)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:103)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:63)
	at org.junit.runners.ParentRunner$4.run(ParentRunner.java:331)
	at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:79)
	at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:329)
	at org.junit.runners.ParentRunner.access$100(ParentRunner.java:66)
	at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:293)
	at org.junit.runners.ParentRunner$3.evaluate(ParentRunner.java:306)
	at org.junit.runners.ParentRunner.run(ParentRunner.java:413)
	at org.apache.maven.surefire.junit4.JUnit4Provider.execute(JUnit4Provider.java:316)
	at org.apache.maven.surefire.junit4.JUnit4Provider.executeWithRerun(JUnit4Provider.java:240)
	at org.apache.maven.surefire.junit4.JUnit4Provider.executeTestSet(JUnit4Provider.java:214)
	at org.apache.maven.surefire.junit4.JUnit4Provider.invoke(JUnit4Provider.java:155)
	at org.apache.maven.surefire.booter.ForkedBooter.runSuitesInProcess(ForkedBooter.java:385)
	at org.apache.maven.surefire.booter.ForkedBooter.execute(ForkedBooter.java:162)
	at org.apache.maven.surefire.booter.ForkedBooter.run(ForkedBooter.java:507)
	at org.apache.maven.surefire.booter.ForkedBooter.main(ForkedBooter.java:495)

@basil basil added ath-fail The acceptance-test-harness suite needs a forward-compatible change and removed ath-successful This PR has successfully passed the full acceptance-test-harness suite labels Jul 12, 2024
@timja timja added ath-successful This PR has successfully passed the full acceptance-test-harness suite and removed ath-fail The acceptance-test-harness suite needs a forward-compatible change labels Jul 13, 2024
@timja
Copy link
Member

timja commented Jul 13, 2024 •
edited
Loading

There is a PR that was waiting for release of this change for that, I think it will work now as well that its merged and not need to wait till Tuesday: jenkinsci/acceptance-test-harness#1584

because of where the change was forward compat was awkward and its doing version detection.

@basil
Copy link
Member

basil commented Sep 5, 2024

Broke org.jenkinsci.plugins.oic.PluginTest#testLastGrantedAuthoritiesProperty.

basil added a commit to basil/oic-auth-plugin that referenced this pull request Sep 5, 2024
@basil
Forward compatibility with jenkinsci/jenkins#7268
1748ffd
@basil basil mentioned this pull request Sep 5, 2024
Merged
basil added a commit to jenkinsci/oic-auth-plugin that referenced this pull request Sep 5, 2024
@basil
Forward compatibility with jenkinsci/jenkins#7268 (#389)
6fdfc07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Kevin-CB Kevin-CB Kevin-CB approved these changes

@janfaracik janfaracik janfaracik approved these changes

@timja timja timja approved these changes

Assignees
No one assigned
Labels
ath-successful This PR has successfully passed the full acceptance-test-harness suite developer Changes which impact plugin developers major-rfe For changelog: Major enhancement. Will be highlighted on the top pct-successful This PR has successfully passed the full plugin-compatibility-test suite ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback security-approved @jenkinsci/core-security-review reviewed this PR for security issues web-ui The PR includes WebUI changes which may need special expertise
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Wadeck @janfaracik @timja @Kevin-CB @basil